PR Review: test: expand coverage from 13% to 55% with 3,719 tests

This is a substantial and well-executed test expansion. The overall direction is correct and the engineering quality is high. Below is detailed feedback organized by category.

Strengths

Shared testDbHelper.js — excellent architecture decision.
Creating a single createTestDb() helper that spins up a uniquely-named fake-indexeddb instance per test (using the dbCounter + Date.now() naming scheme) is exactly the right pattern. It eliminates test-to-test state leakage, mirrors real IndexedDB semantics, and makes the full 17-store schema available to every suite without duplication. The seedStore / readAll utilities are clean and reusable.

Consistent mock-before-import ordering.
Every .real.test.js file correctly hoists jest.mock(...) calls above all imports, which is required for Jest module hoisting to work. This discipline is maintained across all 72 files—a sign of careful authorship.

Good use of factory helpers (makeProblem, makeTagRelationship, etc.).
Per-file factory functions keep test data readable and avoid copy-paste repetition. Defaults are sensible, and ...overrides at the end allows pinpoint customization without verbose full-object literals.

Behavior-level assertions for DB-backed code.
Rather than just checking return values, tests like saveUpdatedProblem actually read back from the store to confirm the write landed (readAll → toHaveLength(1)). This is the right level of integration for a "real DB" test.

Cleanup of dead/skipped tests.
Removing 16 trivial tests, 10 dead skipped tests, and 5 redundant suites is good hygiene. Skipped tests carry a hidden maintenance cost.

Issues and Recommendations

1. testDbHelper.js schema drift risk (medium severity)

The STORES array in testDbHelper.js is a hand-maintained copy of storeCreation.js. If either file diverges—a new store, a new index, a changed keyPath—tests can pass against the wrong schema while production fails.

Recommendation: Import the store definitions directly from storeCreation.js rather than duplicating them:

// test/testDbHelper.js
import { STORE_DEFINITIONS } from '../src/shared/db/core/storeCreation.js';

This eliminates the entire class of drift bugs. If the import path needs adjustment for the test environment, a one-time alias in jest.config.js handles it cleanly.

2. Known transaction bug documented but not tracked (medium severity)

tag_mastery.real.test.js contains this comment:

"The function has a known issue where getLadderCoverage() opens a second transaction inside a loop that already holds an active readwrite transaction on tag_mastery. In fake-indexeddb this causes the write transaction to auto-commit..."

This is documenting a production bug, not a test limitation. The test currently exercises the error-handling path as a workaround. This should be filed as a separate issue and linked here so it is not forgotten after the PR merges.

3. setup.js completely silences console.error during tests

error: (...args) => {
  if (process.env.NODE_ENV === 'test') {
    return; // Only suppress during normal test execution
  }
  ...
}

NODE_ENV is always 'test' when Jest runs, so console.error is a no-op in every test. This can mask real bugs—services that call logger.error() or throw unhandled rejections produce no visible signal during a test run.

Recommendation: Restore console.error to a spy that records calls, or at minimum emit to process.stderr so errors appear in CI output.

4. afterEach in setup.js clears indexedDB._databases via private API

afterEach(() => {
  if (global.indexedDB && global.indexedDB._databases) {
    global.indexedDB._databases.clear();
  }
});

fake-indexeddb manages database lifecycle internally. Clearing _databases directly reaches into a private API that may break across fake-indexeddb version upgrades (the package is already at v6). The closeTestDb helper already closes the connection properly; the global wipe should be removed.

5. structuredClone polyfill loses non-JSON-serializable values

if (!global.structuredClone) {
  global.structuredClone = (obj) => JSON.parse(JSON.stringify(obj));
}

This polyfill silently drops undefined values, corrupts Date objects (converts to strings), and cannot handle Map, Set, RegExp, or circular references. Node 17+ provides a native structuredClone; verify whether the Jest environment already provides it before keeping this polyfill.

6. Coverage thresholds are commented out

// coverageThreshold: {
//   global: { branches: 70, functions: 70, lines: 70, statements: 70 }
// },

Now that coverage has jumped from 13% to 55%, this is the right time to lock in a minimum floor. A threshold of 50% lines would protect the gains from future regression without creating friction.

7. Duplicate mock setup between setup.js and individual test files

setup.js globally mocks logger.js and PerformanceMonitor.js. Many individual test files also mock these same modules. This is harmless (file-level mocks take precedence) but creates redundant noise. Pick one approach and be consistent.

8. getStore mock opens a new transaction per call

getStore: jest.fn(async (storeName, mode = 'readonly') => {
  return db.transaction(storeName, mode).objectStore(storeName);
}),

Each getStore call opens a fresh transaction. If production code relies on a single shared transaction for atomicity or for transaction.oncomplete, the mock behaves differently. This is worth verifying against the real dbHelper.getStore implementation to ensure the mock accurately reflects production behavior.

Minor Notes

• Verify that all .real.test.js files have matching beforeEach/afterEach pairs—a few files may open a testDb without a corresponding closeTestDb call.
• The .real.test.js vs .test.js naming convention is clear and useful; worth documenting in CLAUDE.md.
• The fix(test): resolve setTimeout race condition in recalibrationService commit is a good sign—race conditions fixed rather than masked with fake timers.

Summary

The implementation is solid and moves the project in the right direction. The two highest-priority items before merge are: (1) filing a tracking issue for the calculateTagMastery transaction bug, and (2) considering the schema import consolidation to prevent future drift.

PR Review: test: expand coverage from 13% to 55% with 3,719 tests

This is a substantial PR — 44k lines of new test code across 72 new files. The high-level direction is excellent: moving from brittle mock-based tests to real fake-indexeddb tests is the right approach for a database-heavy codebase. Here is detailed feedback across the key dimensions.

What is done well

testDbHelper.js is a solid shared fixture. The full 17-store schema, unique-per-test DB names (CodeMaster_jest_${dbCounter}_${Date.now()}), seedStore/readAll helpers, and closeTestDb for cleanup are all exactly right. This makes the real-DB tests reproducible and non-interfering.

Consistent test structure. The pattern of jest.mock → imports → factory helpers → beforeEach/afterEach lifecycle → grouped describe blocks is consistent across the new files, which makes the suite easy to navigate.

setup.js improvements. Mocking PerformanceMonitor globally and adding the _testDatabaseActive / IS_BACKGROUND_SCRIPT_CONTEXT flags are clean solutions that avoid test-environment-specific hacks bleeding into production code.

Proper cleanup. afterEach(() => closeTestDb(testDb)) consistently avoids shared state between tests. Clearing indexedDB._databases in the global afterEach is a good backstop.

Issues to address

1. Known production bug documented in tests but not fixed

tag_mastery.real.test.js (lines 8-16) documents a known transaction ordering bug in calculateTagMastery():

getLadderCoverage() opens a second transaction inside a loop that already holds an active readwrite transaction on tag_mastery. In fake-indexeddb this causes the write transaction to auto-commit, so writeMasteryToDatabase() may silently fail.

The test works around this by asserting resolves.toBeUndefined() (i.e., "at least it does not throw"). However, looking at the production code, calculateTagMastery at line 470 still calls getLadderCoverage inside what appears to be an active transaction scope — even though updateTagMasteryRecords was already fixed to fetch ladder coverage before starting its transaction.

The tests here are measuring survivability of a bug rather than correctness. This should either be fixed in production code or the test should explicitly assert the known-broken behaviour and link to a tracking issue.

2. calculateTagMastery tests have low assertion value

Eight of nine calculateTagMastery test cases assert only resolves.toBeUndefined(). For a function that is supposed to write mastery data to the database, none of the tests verify what was actually written. Even given the known transaction issue, a test that seeds data and then reads back the (partial) result would be more meaningful than checking the function does not throw.

3. Coverage thresholds are commented out

jest.config.js has this block permanently disabled:

// Coverage thresholds - disabled to unblock CI
// coverageThreshold: {
//   global: {
//     branches: 70,
//     functions: 70,
//     lines: 70,
//     statements: 70
//   }
// },

Going from 13% to 55% is great progress, but shipping with thresholds permanently disabled means coverage can silently regress. Now that 55% is established, re-enabling thresholds at a conservative 50% would protect against future regressions.

4. console.error is fully suppressed in test mode

In setup.js:

error: (...args) => {
  if (process.env.NODE_ENV === "test") {
    return; // Only suppress during normal test execution
  }
  originalError.call(console, ...args);
},

This drops all console.error calls during tests, which hides real test failures and unexpected errors. The original intent (suppressing JSDOM navigation warnings) is already handled by the filter earlier in the file. Consider narrowing this override to only suppress the specific patterns that actually need suppression.

Things to consider

5. Naming: .real.test.js vs the existing .test.js convention

The real suffix clearly communicates "uses real fake-indexeddb", which is useful. However, the coexistence of files like tag_mastery.test.js and tag_mastery.real.test.js covering overlapping functions (getTagMastery, upsertTagMastery, calculateTagSimilarity) can create confusion about which file is authoritative. It may be worth a follow-up to consolidate the remaining overlapping test files.

6. Handler tests are named .real but mock all services

sessionHandlers.real.test.js and the other handler tests in background/handlers/__tests__/ have the .real suffix but mock all service dependencies (SessionService, StorageService, etc.). They are unit tests of handler glue code — which is fine and useful — but the naming is slightly misleading. Consider either renaming to .unit.test.js or clarifying the naming convention in a comment.

7. ErrorReportService.real.test.js uses hand-rolled mock IDB

This file manually implements createAutoRequest / createMockStoreAndDb instead of using the new testDbHelper.js. This is inconsistent with the rest of the PRs approach and means these tests do not exercise real IndexedDB transaction semantics. It would benefit from being migrated to testDbHelper in a follow-up.

Summary

The PR is a clear net positive for the codebase. The items above — particularly the production bug in calculateTagMastery and the suppressed console.error — should be addressed before merge. The rest are good candidates for follow-up issues.

Generated with Claude Code

PR Review Summary

Two detailed reviews are already posted above. This comment consolidates the key action items.

Must-address before merge

1. Production bug in calculateTagMastery

The transaction ordering issue — opening a second transaction inside an active readwrite transaction on tag_mastery — is documented in the tests but not fixed in production code. The current tests assert resolves.toBeUndefined() (survivability, not correctness). File a tracking issue and link it to this PR so it is not lost after merge.

2. console.error is fully suppressed in tests

In setup.js, the check if (process.env.NODE_ENV === 'test') { return; } silences all errors during every test run since NODE_ENV is always 'test' in Jest. This hides real failures. Narrow the suppression to only the specific JSDOM navigation-warning pattern it was originally written for.

3. Coverage thresholds remain commented out

With 55% established, re-enable coverageThreshold at a conservative floor (e.g. 50% lines/statements) so future regressions are caught in CI rather than silently landing on main.

Good to address in follow-up issues

• Schema drift risk — testDbHelper.js hand-copies the 17-store schema from storeCreation.js. Importing the definitions directly eliminates an entire class of drift bugs.
• _databases.clear() private API — The global afterEach clears indexedDB._databases directly, which is a private fake-indexeddb internal. The closeTestDb call is sufficient; the global wipe should be removed.
• structuredClone polyfill — The JSON.parse/JSON.stringify fallback silently drops undefined values, corrupts Date objects, and fails on Map/Set/circular refs. Verify whether Node 17+ already provides native structuredClone in the Jest environment.
• Handler tests named .real but mock all services — Tests under background/handlers/tests/ carry the .real suffix but mock every service dependency. Either rename to .unit.test.js or document the naming convention in CLAUDE.md.
• ErrorReportService.real.test.js uses hand-rolled mock IDB — Inconsistent with the shared testDbHelper.js approach used everywhere else; migrate in a follow-up.

Overall assessment

The PR is a clear net positive. The three must-address items are the main blockers; the rest are suitable for follow-up issues.

Generated with Claude Code